Skip to content

Distance API added#219

Merged
cammace merged 1 commit intomasterfrom
cam-distance
Nov 30, 2016
Merged

Distance API added#219
cammace merged 1 commit intomasterfrom
cam-distance

Conversation

@cammace
Copy link

@cammace cammace commented Nov 26, 2016

Closes #168

Adds the Distance API to mapbox-java. Still need to add test and clean up some of them code. Example activity can be found in testapp:

cc: @mapbox/android

@cammace cammace added this to the v2.0.0 milestone Nov 26, 2016
@mention-bot
Copy link

@cammace, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zugaldia to be a potential reviewer.

@cammace
Copy link
Author

cammace commented Nov 29, 2016

Build is failing since I upgraded from 4.2.0-beta.1 to 4.2.0-beta.5 which includes libjava in it. Dex files were interfering. I added

exclude group: 'com.mapbox.mapboxsdk', module: 'mapbox-java-services'

@ivovandongen can you review this one?

@ivovandongen
Copy link
Contributor

@cammace The Distance related code seems good. There is a lot in there that doesn't seem to belong to this PR though (images, dependency updates). Would be great to separate these for easier reviews/traceability.

@cammace
Copy link
Author

cammace commented Nov 30, 2016

Cleaned up PR and moved some changes into #223.

setSupportActionBar(toolbar);
getSupportActionBar().setDisplayHomeAsUpEnabled(true);

coordinates = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initialisation can go into the field declaration as it's only done once anyway. That way you can make them final and there's no chance of null pointer exceptions (when refactoring)

@Override
public void onMapClick(@NonNull LatLng point) {

features = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace with clear and initialise in declaration


features.addAll(mapboxMap.queryRenderedFeatures(pixel, "circle-layer"));

if (features.size() <= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to do the opposite here; size() > 0 -> callDistanceApi() and have just a single exit point in the method

for (int i = 0; i < restaurants.size(); i++) {
// Get the json object and replace the duration property with the updated time. We convert the API's
// second value to minutes.
JsonObject jsonObject = restaurants.get(i).getProperties().getAsJsonObject();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can never be null?

restaurants.get(i).getStringProperty("name") + "-layer-name",
restaurants.get(i).getStringProperty("name") + "-source"
).withProperties(
PropertyFactory.textSize(14f),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be shortened with static import

*/
@Override
public Geometry deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context)
throws JsonParseException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation looks a bit off here. Or is that the new code style?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the new style, line 31 is over the max line length so I broke it up by adding throws on the line below.

@cammace cammace merged commit d54d576 into master Nov 30, 2016
@cammace cammace deleted the cam-distance branch November 30, 2016 17:51
@zugaldia zugaldia mentioned this pull request Feb 10, 2017
9 tasks
@zugaldia zugaldia mentioned this pull request Feb 22, 2017
9 tasks
@zugaldia zugaldia mentioned this pull request Mar 9, 2017
9 tasks
@zugaldia zugaldia mentioned this pull request Mar 17, 2017
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants